Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: zero gas fee poc #29

Draft
wants to merge 4 commits into
base: basechain/develop
Choose a base branch
from
Draft

Conversation

dudong2
Copy link
Collaborator

@dudong2 dudong2 commented Dec 5, 2024

Closes: #XXX

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@dongsam
Copy link
Member

dongsam commented Dec 6, 2024

LGTM and sufficient and well-designed as a PoC

  • Currently, zero-fee registration is allowed for specific Contract * Signature, but if restrictions could also be applied to the from address, it would enhance spam prevention by enabling zero-fee application for Contract * Signature * from_address combinations
  • Once the requirements and use case details are finalized, it can be determined whether from address validation needs to be included, and the implementation can then be refined for production

Copy link
Collaborator

@cloudgray cloudgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments.

x/evm/types/zero_gas_config.pb.go Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/tx.proto Outdated Show resolved Hide resolved
string contract_address = 1 [(gogoproto.moretags) = "yaml:\"contract_address\""];
// signatures is a list of function signatures that will have zero gas fee
// when called on the specified contract_address
repeated string signatures = 2 [(gogoproto.moretags) = "yaml:\"signatures\""];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using the decoded string version of the function signature?

Here are three reasons for this suggestion:
1. When updating, users must manually calculate the function signature, which can be cumbersome.
2. When retrieving a stored function signature, the result is a 4-byte value like “0xa9059cbb”. Over time, it may become difficult to remember which function this corresponds to.
3. Although the probability is very low, there is a chance that 4-byte function signatures could collide. Since Solidity compilers do not check for such collisions, this could potentially be exploited maliciously.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. However, there is an inefficiency in having to calculate the signature every time if it is used frequently. It's not a huge overhead, but the inefficiency needs to be addressed.

Copy link
Collaborator

@cloudgray cloudgray Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dudong2 Then, How about below?

  1. get input as string version
  2. store both of string and 4bytes
  3. verify using 4bytes

Inefficient hashing will not be needed for verification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants